Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have the measurement unit depend on the timezone (which is based on the metadata coordinates). #111

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nicomiguelino
Copy link
Contributor

@nicomiguelino nicomiguelino commented Aug 15, 2023

Overview

Screenshots

@StanIwan & @vpetersson Here are some screenshots of the weather app (across various locations).

Toronto

image

Vancouver

image

Mock data used

Here's a snippet from my mocked screenly.js:

const getCoordinates = (area) => {
  switch (area) {
    case 'San Francisco':
      return [
        '37.7749',
        '-122.4194'
      ]
    case 'Manila':
      return [
        '14.5995',
        '120.9842'
      ]
    case 'Dijon':
      return [
        '47.3220',
        '5.0415'
      ]
    case 'New York':
      return [
        '40.7128',
        '-74.0060'
      ]
    case 'Toronto':
      return [
        '43.6532',
        '-79.3832'
      ]
    case 'Vancouver':
      return [
        '49.2827',
        '-123.1207'
      ]

    default:
      return []
  }
}

const screenly = {
  metadata: {
    coordinates: getCoordinates('Vancouver'),
    hardware: 'Screenly Player Max',
    hostname: 'srly-abcdefghijklmno',
    location: 'San Francisco, USA',
    screen_name: 'Edge App Test',
    screenly_version: 'v1.0.0',
    tags: [
        'All Screens'
    ]
  },
  settings: {
    disable_analytics: false,
    ga_api_key: 'mcd0n4ld5',
    sentry_id: 'p4nd4expr355',
    // Make sure to obfuscate the OpenWeatherMap API key as well.
    openweathermap_api_key: '[redacted]',
  }
}

@vpetersson
Copy link
Contributor

As per @StanIwan's comment on Slack, can you double check this with some locations in Canada to ensure it is using the right unit?

@nicomiguelino
Copy link
Contributor Author

@vpetersson, it seems that the libraries being used are quite smart. Thanks @StanIwan for the feedback!

@StanIwan
Copy link
Contributor

@nicomiguelino I might be terribly mistaken here, but shouldn't Toronto be in metric (C) instead of imperial units (F)?

@nicomiguelino
Copy link
Contributor Author

nicomiguelino commented Aug 22, 2023

Thanks, @StanIwan. I did a quick research as well to supplement your comments. It seems that Canada is using Celsius.
Here's some reference that I found - https://www.learnalberta.ca/content/kes/pdf/or_cf_math_ss_a_03_temp.pdf. (Don't have a more credible source at the moment).

Requesting for another review, @StanIwan and @vpetersson...

let timezones = moment.tz.zonesForCountry(country)

if (country === "BS") {
timezones = timezones.filter(tz => tz !== 'America/Toronto')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no. This is a slippery slope. Please find a library that can do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason we want to tie time units to timezone instead of country?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason we want to tie time units to timezone instead of country?

how are timezone and countries correlated? A country might have a lot of timezones, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, according to ChatGPT at least, there is no country that uses both. Thus mapping it to country would be fine. Yet, I want a library for this rather than a hard coded list (which can get long i guess).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants